Allow Steps to Accept Traversal#3458
Conversation
|
|
||
| The traversal-accepting forms of `has()` allow for dynamic property comparisons. Rather than providing a literal value, | ||
| a child traversal is supplied and its first result is used as the comparison value. This follows the same first-result | ||
| semantics as `by(traversal)`. The child traversal must be read-only - mutating steps are rejected. |
There was a problem hiding this comment.
The child traversal must be read-only - mutating steps are rejected.
Don't really like dashes in our docs this way, how about: "The child traversal cannot include mutating steps like addV() or mergeE(). If these steps are present, they will be rejected at runtime with a XYZException."
| <2> Find projects having two or more contributors. | ||
| <3> Find projects whose contributors average age is between 30 and 35. | ||
|
|
||
| The `is()` step also supports predicates that contain traversal arguments for dynamic threshold comparison. |
There was a problem hiding this comment.
any reason to not just fold this back into the previous examples? i'm not sure it needs to stand out as a separate thing.
| <7> The label value can be specified as a property only at the time a vertex is added and if one is not specified in the addV() | ||
| <8> If you pass a `null` value for the Map this will be treated as a no-op and the input will be returned | ||
|
|
||
| The `property()` step also accepts a traversal that produces a `Map` of key-value pairs to set as properties. The |
There was a problem hiding this comment.
again, don't like the dash this way in our docs:
The child traversal must be read-only - mutating steps are rejected.
you could use the other verbiage i had or some variation of it.
If the traversal does not produce a
Map, the result is rejected.
rejected when and in with what exception?
|
|
||
| [gremlin-groovy,modern] | ||
| ---- | ||
| g.V(4).property(__.V(1).project('friendCount','createdSoftware').by(__.out('knows').count()).by(__.out('created').values('name'))) <1> |
There was a problem hiding this comment.
please format the query to make this easier to read.
| [gremlin-groovy,modern] | ||
| ---- | ||
| g.V(4).property(__.V(1).project('friendCount','createdSoftware').by(__.out('knows').count()).by(__.out('created').values('name'))) <1> | ||
| g.V(4).valueMap('friendCount','createdSoftware') |
There was a problem hiding this comment.
maybe better to show the whole vertex with all properties to demonstrate we augmented an existing vertex that had properteis?
| g.V(4).valueMap('friendCount','createdSoftware') | ||
| ---- | ||
|
|
||
| <1> Set two properties on vertex 4 (josh) from a Map produced by a child traversal. The `project()` step builds a Map |
There was a problem hiding this comment.
if you get a better multi-line formatting as i mentioned above, then this description can be broken into smaller bits for folks to understand more easily
| <1> Normally the `V()`-step will iterate over all vertices. However, graph strategies can fold ``HasContainer``'s into a `GraphStep` to allow index lookups. | ||
| <2> Whether the graph system provider supports mid-traversal `V()` index lookups or not can easily be determined by inspecting the `toString()` output of the iterated traversal. If `has` conditions were folded into the `V()`-step, an index - if one exists - will be used. | ||
|
|
||
| The `V()` step also accepts a traversal argument. The child traversal is evaluated and its results are used as the |
There was a problem hiding this comment.
again, i feel like this doesn't need to be an "also" in how this step works. just integrate it into the other examples and content more seamlessly.
| <8> Marko is younger than josh, but josh knows someone equal in age to marko (which is marko). | ||
| <9> The "age" property is not <<by-step,productive>> for all vertices and therefore those values are filtered. | ||
|
|
||
| The `where()` step also supports predicates that contain traversal arguments. When a predicate contains a child |
There was a problem hiding this comment.
again, as with other comments, let's integrate this more seamlessly into the content. Also, perhaps where is a case where we could come up with a more complex example since it's a more advanced step.
| partial work is discarded if the user forgets to call `commit()`. In Java (both remote and embedded mode), the behavior | ||
| can still be overridden via `tx.onClose(Transaction.CLOSE_BEHAVIOR.COMMIT)`. The non-Java GLVs do not support | ||
| configuring close behavior and always rollback. | ||
| ==== Traversal-Accepting Steps |
There was a problem hiding this comment.
Need a catchier title here. "Expanding Dynamic Arguments"??
| configuring close behavior and always rollback. | ||
| ==== Traversal-Accepting Steps | ||
|
|
||
| Steps and predicates that previously only accepted literal values now accept child traversals resolved per-traverser |
There was a problem hiding this comment.
Folks have been waiting on this feature for a long time. It needs a strong introduction with some history about the challenges folks had with the old way to do some of this stuff. Show some comparisons demonstrating the old way versus the ease of the new way.
Stuff like the following is instructional details and doesn't really belong here:
Child traversals take only the first result (consistent with
by(traversal)semantics). Forwithin()/without(),
usefold()to collect multiple values into a list.
i think we should also be telling users in this section what patterns this feature allows them to replace to make their code perform better, read better, etc.
|
|
||
| All child traversals must be read-only. Mutating steps (`addV`, `addE`, `drop`, `property`) inside child traversals | ||
| are rejected at construction time with `IllegalArgumentException`. A `ChildTraversalVerificationStrategy` provides | ||
| additional safety at strategy time. |
There was a problem hiding this comment.
Aren't there some JIRAs for this to reference?
|
There are some docs here that cover the feature, but what I don't see are updates to existing docs that use old patterns that are no longer relevant and that we should no longer promote. Has that kind of search been done? |
| * For any step containing a child traversal argument, serializing to GremlinLang and parsing back | ||
| * produces a structurally equivalent traversal. | ||
| */ | ||
| public class GremlinLangTraversalRoundTripTest { |
There was a problem hiding this comment.
Good tests, though i can't help wondering if this is just agent generated stuff that is either:
- already being covered elsewhere, meaning these tests should be there or
- covered by other processes making this set of unit tests kinda redundant or
- we don't have enough round-trip tests to cover so many other interesting cases - meaning we should improve this test dramatically to cover lots of cases.
There was a problem hiding this comment.
I've gone with option 3, and built this class out to be more well rounded. It's not a single parameterized test with 45 to give more complete coverage. The main gap that this test fills is validating GremlinLang strigification/parsing roundtrip equality. To an extent this was already covered through feature tests which are parsed from the gherkin source, and then stringified for remote execution, however that only verifies the traversal produces the expected results, which is weaker than asserting equal traversals through the round trip.
| * to an empty collection rather than short-circuiting), scalar predicates remain resolved-empty, and the | ||
| * shared-state safety of resolving the same predicate across many traversers sequentially. | ||
| */ | ||
| public static class EmptyResolutionAndConnectiveTest { |
There was a problem hiding this comment.
I think there's room for more complex test cases here with embedded ands/ors. come up with some wild scenarios
There was a problem hiding this comment.
This has been migrated to PTest and now includes more deeply nested ands of ors and ors of ands.
| /////////////////////////// | ||
|
|
||
| private static boolean processHasStep(final HasStep<?> step, final Traversal.Admin<?, ?> traversal) { | ||
| if (step.getHasContainers().stream().anyMatch(HasContainer::hasTraversal)) return false; |
There was a problem hiding this comment.
we changed this strategy, but i don't see any unit tests here: https://github.com/apache/tinkerpop/blob/master/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/InlineFilterStrategyTest.java
i think it would be good to add some cases demonstrating these shortcircuit points. i think i'd like more inline code comments around the short circuiting. Update to the class javadoc also seems like a good idea.
There was a problem hiding this comment.
I've updated the javadoc and added additional test cases.
| * and single-value predicate rejection of multiple traversal results. | ||
| */ | ||
| @RunWith(Enclosed.class) | ||
| public class PTraversalTest { |
There was a problem hiding this comment.
While we have a good body of tests here we have none in PTest that carry Traversal. I think that if you look at PTest we cover cases like clone() and negate operations which I don't find coverage for here. That feels like a testing gap.
There was a problem hiding this comment.
I've opted to fold all of these cases directly into PTest. This picks up the clone and negate coverage for free and centralizes all P cases in one place. One noteworthy change is that Traversal-bearing predicates in PTest get resolved with a dummy Traverser, and they bypass the setValue() manipulation tests as the value is nonsense prior to calls to resolve()
| package org.apache.tinkerpop.gremlin.process.traversal.step; | ||
|
|
||
| /** | ||
| * Marker interface for steps that accept user-supplied child traversals embedded as predicate or lookup |
There was a problem hiding this comment.
Marker interface for steps that accept user-supplied child traversals embedded as predicate or lookup arguments
I'm not sure that reads right - " Marker interface for steps that accept child traversals used to provide dynamic values to {@link P} or to the step itself"
There was a problem hiding this comment.
I think this points to a larger branding/framing problem with this interface. I've renamed it ReadOnlyTraversalParent and updated the javadoc. See below thread for more details.
| * | ||
| * @since 4.0.0 | ||
| */ | ||
| public interface AcceptsChildPredicateTraversal { |
There was a problem hiding this comment.
I don't really like this name: AcceptsChildPredicateTraversal . It's plainly descriptive but I'd like something more functional. How about: DynamicSelecting? Meaning the step is one that allows for dynamic selection of a value as an argument. This differs from a step that happens to already take Traversal in that the implication of "Selecting" is that it is not allowing just any Traversal - its is for selection only (i.e. no mutations). It's late...maybe a better name is out there along those lines, but AcceptsChildPredicateTraversal is not it imo.
There was a problem hiding this comment.
I've opted to go in a different direction on this one, renaming it to ReadOnlyTraversalParent extends TraversalParent. I added the extends TraversalParent as I don't think this marker interface makes any sense for steps which are not already TraversalParents. I also think that this new framing is easier to understand, it is a TraversalParent with a ReadOnly restriction applied.
| Prior to 4.0, comparing a traverser's value against a dynamically computed reference required verbose workarounds with | ||
| `where()`, `select()`, and labeled steps. For example, finding all people older than "marko" previously required: | ||
|
|
||
| [source,groovy] |
There was a problem hiding this comment.
why are none of the examples actually executed in the Console so that we can see the results?
There was a problem hiding this comment.
The upgrade docs examples should remain static such that they remain as-is following future releases. I don't think the results are particularly interesting for these traversals, however I can add static emulated console output.
There was a problem hiding this comment.
yes, i'm referring to executed in the console with static results. i don't think we've ever used "interesting" or any other reason as criteria for including console output. it's just mostly what we do. we might truncate long results or opt to not display results for GLV related examples (because javascript for example wouldn't make sense in this context), but we generally demonstrate by way of the console.
There was a problem hiding this comment.
I've updated the examples to include the console prefixes and output.
| select("p").values("name") | ||
| ---- | ||
|
|
||
| This pattern was unintuitive and could not easily compose with other predicates. With traversal-accepting arguments, |
There was a problem hiding this comment.
"unintuitive" - better to say, "This pattern required several coordinated step labels and where() compartison" and let the reader decide if it is unintuitive or not.
|
VOTE +1 |
|
|
||
| In addition to literal values, `has()` accepts child traversals for dynamic comparisons. The child traversal is | ||
| evaluated per-traverser and its first result becomes the comparison value, consistent with `by(traversal)` semantics. | ||
| The child traversal cannot include mutating steps like `addV()` or `property()`; if present, they are rejected with |
There was a problem hiding this comment.
The child traversal cannot include mutating steps like
addV()orproperty(); if present, they are rejected with anIllegalArgumentException.
No semi-colons. Use a proper transition word or two sentences.
There was a problem hiding this comment.
This section has been stripped down as the examples have been folded into the main has() examples section. The important context is now captures in the numbered callouts from the specific has(traversal) examples.
|
|
||
| ==== Expanding Dynamic Arguments to Additional Steps | ||
|
|
||
| Prior to 4.0, comparing a traverser's value against a dynamically computed reference required verbose workarounds with |
There was a problem hiding this comment.
"verbose workarounds" - is it right to describe them as "workarounds"? i mean, these were just the semantics at hand and they were designed with purpose. I think the intro needs rewording.
| g.V(4).property(__.V(1).project("friendCount").by(__.out("knows").count())) | ||
| ---- | ||
|
|
||
| See <<has-step,has()>>, <<v-step,V()>>, <<property-step,property()>>, <<is-step,is()>>, and |
There was a problem hiding this comment.
These should be linked as part of the "See:" below.
…-label predicates
WherePredicateStep previously treated traversal-bearing predicates and
scope-label predicates as mutually exclusive paths. This broke mixed
ConnectiveP cases like P.gte(__.constant(0)).and(P.neq("a")) where
one child is a traversal and the other references a scope label.
Now both resolution paths run when needed: P.resolve() handles
traversal children while setPredicateValues() handles scope-label
children (skipping traversal-bearing leaves). Remove outdated javadoc
from GraphTraversal V(Traversal) and E(Traversal) mid-traversal forms.
Assisted-by: Kiro:claude-opus-4.6
The validation is specifically about enforcing read-only (no mutating steps) in child traversals, not something general about child traversals. Rename to distinguish from the unrelated ReadOnlyStrategy which makes the entire graph read-only. Assisted-by: Kiro:claude-opus-4.6
Make it extend TraversalParent so implementors no longer need to declare both. Simplifies the ReadOnlyChildVerificationStrategy instanceof check and removes redundant imports/declarations. Assisted-by: Kiro:claude-opus-4.6
The .NET GraphTraversal and __ classes were missing ITraversal overloads for HasKey, HasValue, and HasLabel, causing compilation failures in the integration tests. Assisted-by: Kiro:claude-opus-4.6
- Integrate traversal examples into existing step sections rather than standalone 'also supports' blocks (has, is, property, V, where) - Replace dashes with explicit exception names (IllegalArgumentException, IllegalStateException) - Use realistic V(traversal) example with inject/select pattern - Fold property(traversal-map) example into the main code block - Format multi-line property() query for readability - Upgrade doc: rename to 'Expanding Dynamic Arguments', add before/after comparison, add JIRA links, complete affected steps list Assisted-by: Kiro:claude-opus-4.6
New entries for E(), has(), hasId(), hasKey(), hasLabel(), hasValue(), is(), property(), V(), and where() documenting their traversal-accepting argument forms and read-only child traversal constraints. Assisted-by: Kiro:claude-opus-4.6
- InlineFilterStrategy: update class javadoc, add inline comments for traversal-bearing short-circuits, add test cases proving exclusion - choose(P): add javadoc noting traversal-bearing predicates throw IllegalArgumentException (GraphTraversal and option()) - P.java: fix wording 'all values' -> 'each value' - ReadOnlyChildValidator: include mutating step name and parent traversal in error message for easier debugging Assisted-by: Kiro:claude-opus-4.6
- HasTraversal.feature (29 scenarios) → Has.feature - IsTraversal.feature (20 scenarios) → Is.feature - WhereTraversal.feature (10 scenarios) → Where.feature - VETraversal.feature → Vertex.feature (6) + Edge.feature (4) - PropertyTraversal.feature → renamed to Property.feature (canonical) - ReadOnlyChildVerification.feature → moved to integrated/ directory with @StepClassIntegrated tag (strategy test, not step test) - Remove incorrect @GraphComputerVerificationMidVNotSupported from g_V_hasXname_notXidentityXX (no mid-V in that traversal) Assisted-by: Kiro:claude-opus-4.6
- GremlinLangTraversalRoundTripTest: convert to parameterized test with 45 cases covering all traversal argument forms (has, is, where, V, E, property, hasId, hasLabel, hasKey, hasValue, ConnectiveP, within/without) - PTraversalTest: add ComplexConnectiveTest with deeply nested and/or combinations including empty-resolution edge cases - PTraversalTest: add CloneAndNegateTest verifying clone/negate preserve child traversals correctly across scalar, collection, and connective predicates - Format long Gremlin in feature tests for readability Assisted-by: Kiro:claude-opus-4.6
Merge all traversal-bearing predicate tests into PTest so that all P behavior is covered in one place. Traversal predicates now participate in the existing parameterized test (shouldTest), gaining automatic clone/negate/P.not coverage. Resolution is handled by a conditional resolve() call for traversal-bearing predicates. Inner classes reorganized as: - TraversalDetectionTest: hasTraversal() accuracy - TraversalResolutionTest: resolve semantics for scalar/collection - TraversalConnectiveTest: and/or/not with nested traversals - TraversalCloneAndNegateTest: clone/negate preserve traversal state Assisted-by: Kiro:claude-opus-4.6
Upgrade doc: - Replace 'verbose workarounds' with neutral language - Replace 'unintuitive' with factual description - Move JIRA links into See: section - Add summarizing migration guidance paragraph Reference doc: - Replace semicolon with two sentences (has() callout) Semantics docs: - E(): add List form to syntax and arguments - V(): add List form to syntax and arguments - has(): expand with T accessor forms, Map domain detail, null semantics - hasId(): add List form to syntax and arguments Gherkin tests: - Add select()-based child traversal test to Has.feature Assisted-by: Kiro:claude-opus-4.6
PTest: the migrated take-first-result tests used inject(1,2,3) whose output order mixes with the seed traverser. Restore union(constant, constant) for deterministic first-result ordering. InlineFilterStrategyTest: correct expectations for traversal-bearing has-steps. filter() unwrapping still applies, and and-decomposition produces two separate HasSteps (the no-merge guard prevents folding the traversal-bearing container into an adjacent HasStep). Verified: full gremlin-core suite passes (9276 tests, 0 failures). Assisted-by: Kiro:claude-opus-4.6
The VETraversal split (integrating V/E scenarios into Vertex/Edge.feature)
left structural artifacts from line-range slicing: dangling
@GraphComputerVerification tags, an orphan result row, and missing blank
lines between scenarios. These caused Gherkin parse failures.
Also replace the mislabeled sack() scenario in Has.feature (which actually
used select()) with a real sack()-based test:
g.withSack(29).V().has('age', P.gt(__.sack())).values('name')
Verified: TinkerGraphParameterizedFeatureTest passes (2280 tests, 0 failures).
Assisted-by: Kiro:claude-opus-4.6
…arios g_VXVXvid1X_idX_name and g_EXVXvid1X_outE_idX use a child traversal containing __.V(vid1), which becomes an unsupported mid-traversal V() on GraphComputer. These two start-step scenarios lacked the @GraphComputerVerificationMidVNotSupported tag in the original VETraversal.feature. Verified: TinkerGraphComputerFeatureTest passes (0 errors) and both scenarios still pass on the standard engine. Assisted-by: Kiro:claude-opus-4.6
Previously resolve() took the first result from each child traversal and flattened any collection results into one combined membership set. This didn't match literal within/without semantics, where unfolding only happens for a single collection argument. New behavior (mirrors literal forms, gated on isCollection + first-result): - within(t): first result acts like within(Collection) - a Collection result unfolds into the membership set, otherwise it is one value - within(t1, t2, ...): mirrors within(v1, v2, ...) - each first result is one membership element, no unfolding - scalar predicates (eq/gt/...) never unfold Rewrote the multi-source feature scenarios to the union(...).fold() single-traversal idiom (per review), preserving their expected results. Added PTest cases covering single-traversal unfold vs multi-traversal no-unfold. Updated reference docs and P factory javadocs. Regenerated the GLV feature test data (translations.json, gremlin.py, gremlin.js, gremlin.go, Gremlin.cs) so they stay in sync with the updated .feature sources. Verified: gremlin-core (9284), TinkerGraph standard + computer feature suites all pass. Assisted-by: Kiro:claude-opus-4.6
The predicate factory methods (P.eq/gt/within/etc.) accept Traversal, and __.foo() is already a GraphTraversal, so the .asAdmin() suffix on child traversal arguments is unnecessary and unrealistic for how users write these. Stripped them from PTest, GremlinLangTraversalRoundTripTest, and one case in TinkerGraphStepStrategyTraversalTest. Kept the asAdmin() calls that are genuinely required: Admin-API access (getGremlinLang, addStep) and Admin-typed variables/parameters. Assisted-by: Kiro:claude-opus-4.6
Addresses review feedback asking to see results for the examples. Per the discussion, the examples remain static (not executed at doc-build time) so they stay stable across releases, but now show gremlin> prompts and ==> output captured from the modern graph. Also updated the multi-source within() example to the union(...).fold() single-traversal form, consistent with the corrected within/without semantics. Output verified by running each example through the Gremlin Console against TinkerFactory.createModern(). Assisted-by: Kiro:claude-opus-4.6
The has() traversal-argument examples were still in a separate code block with their own prose, unlike is()/where()/property()/V() which integrate their traversal examples into the main block. Folded the three examples in as callouts <11>-<13> and condensed the explanation into the callout and a concise NOTE (read-only constraint, P.eq wrapping, pointer to A Note on Predicates). Removed the semicolon per earlier review feedback. Verified the examples execute against the modern graph. Assisted-by: Kiro:claude-opus-4.6
The traversal examples leaned on the fixed __.V(1).values(...) lookup pattern. Added two examples that show the comparison value being computed dynamically, which better conveys what traversal arguments enable: - is(): filter ages above the average person's age, with the threshold computed by a child traversal (mean()) - has(): filter people older than a threshold carried in the traverser's sack(), showing the value can come from traverser state rather than the graph Both verified against the modern graph (is -> [32,35], has -> [josh,peter]). Assisted-by: Kiro:claude-opus-4.6
Correcting a misread of the review comment. The comment asked for the reference-doc cross-links (has(), V(), property(), is(), where()) to be folded into the See: block alongside the JIRA links, rather than left as a separate standalone 'See ...' sentence. There is now a single unified See: section containing both the reference links and the JIRA links. Assisted-by: Kiro:claude-opus-4.6
ReadOnlyChildVerification.feature now carries the common strategy-test
pattern used by peer integrated features - @WithReadOnlyChildVerificationStrategy
scenarios that invoke the strategy explicitly via withStrategies() (a
pass-through, a valid read-only child, and a rejected mutating child).
completed the @GraphComputerVerificationMidVNotSupported audit.
- Removed the tag from the sack() scenario in Has.feature (start V() only,
no mid-traversal V(); verified it runs on GraphComputer).
- Removed the tag from all ReadOnlyChildVerification rejection scenarios and
corrected the rationale comment: the strategy rejects at strategy-application
time with the same 'mutating step' message under both OLTP and OLAP (verified),
so no exclusion is needed. Kept the tag only on the valid scenarios that
actually execute a child V() lookup.
Added the missing select()/sack() cases to Is.feature (select() carries a
mid-V via as('a').V() and is tagged; sack() has no mid-V and is untagged).
Regenerated GLV feature data to stay in sync.
Verified: TinkerGraph standard (2189) and computer (3238) feature suites pass.
Assisted-by: Kiro:claude-opus-4.6
The scaffolding scenarios used withStrategies(ReadOnlyChildVerificationStrategy), which broke the gremlin-server Groovy script path (No such property) and would also break the GLV radish tests: the generated gremlin.py/js/go/cs emit a ReadOnlyChildVerificationStrategy() constructor that no GLV defines. Making the scaffolding pass everywhere would require registering this internal default verification strategy as a user-facing, serializable strategy across CoreImports, GraphSONModule, and all four GLVs - a public-API expansion that needs a deliberate decision. Reverted the three withStrategies scenarios and the CoreImports change. Retained the in-scope, verified work: the GraphComputer tag audit (removed over-tags from the rejection scenarios with corrected rationale) stays. Verified: TinkerGraph standard (2186) and computer (3233) feature suites pass. Assisted-by: Kiro:claude-opus-4.6
Gives the strategy equivalent support to StandardVerificationStrategy so it can be referenced via withStrategies()/withoutStrategies() consistently across all languages, and restores the common strategy-test scaffolding (#22). - CoreImports: import + CLASS_IMPORTS so the Groovy script engine resolves it - GraphSONModule: register in all four strategy lists for serialization - GLVs: define the strategy in Python (strategies.py), Go (strategies.go), JavaScript (traversal-strategy.ts), and .NET (new ReadOnlyChildVerificationStrategy.cs), mirroring StandardVerificationStrategy - ReadOnlyChildVerification.feature: restore the withStrategies scaffolding scenarios (pass-through, valid read-only child, rejected mutating child) - Regenerated GLV feature data The Go translator lists are intentionally untouched: the config-less withStrategies(ReadOnlyChildVerificationStrategy) form takes the simple translation path, same as ReadOnlyStrategy. Verified: TinkerGraph standard (2189) and computer (3238) feature suites pass; gremlin-server Groovy, GraphSON-lang, and GraphBinary-lang feature tests pass for the scaffolding scenarios (3/3 each). Assisted-by: Kiro:claude-opus-4.6
Reconcile existing documentation with the new support for traversal arguments on steps and predicates so the docs no longer promote or describe superseded patterns. - traversal-induced-values recipe: replace the two-traversal variable lookup and the as()/where().by() label-comparison with the recommended single-traversal predicate form (has(key, gt(traversal))), and modernize the "movies liked by all friends" example to use is(eq(traversal)) instead of as()/where(eq()) count coordination. - anti-patterns recipe: remove the "has() and Traversal Arguments" section, which documented the old behavior that traversal arguments do not resolve to comparison values. That behavior is now reversed. - the-traversal reference: drop the now-dangling link to the removed anti-pattern from the has() step's additional references. Assisted-by: Kiro:claude-opus-4.8
Move read-only child traversal validation into the existing step loop in StandardVerificationStrategy, eliminating a separate strategy pass. Also validate global children for future-proofing. - Remove ReadOnlyChildVerificationStrategy class and its test - Remove GLV strategy classes (.NET, Go, JS, Python) and test translations - Fold feature test scenarios into StandardVerificationStrategy.feature - Rewrite feature tests to reduce @GraphComputerVerificationMidVNotSupported usage: use constant() for simple predicate tests, keep mid-V only where graph traversal is the test's purpose - Add V(Traversal) javadoc reference link - Expand and fix semantics doc entries for all new steps Assisted-by: Kiro:claude-opus-4.6
The 10 new step entries added by this branch (E, has, hasId, hasKey, hasLabel, hasValue, is, property, V, where) used the older Java-flavored conventions. Master has since adopted GType-aligned type names, TypeScript parameter notation in `*Syntax:*` lines, and language-neutral error categories. Update the branch's new entries to match so the merge to master lands clean. - Convert `*Syntax:*` lines to TypeScript-style `name: TYPE` form. Use GType uppercase for primitives (`STRING`, `any`, etc.), PascalCase for TinkerPop concept types (`Traversal`, `Cardinality`, `T`, `P`). - Replace Java exception class references (`IllegalArgumentException`, `IllegalStateException`) in Considerations and Exceptions with the TinkerPop error categories (`Argument Error`, `State Error`). - Rename `Map` to `MAP` (GType) and `String` to `STRING` in prose where the reference is to the type. - Replace the Java `Collection` references in argument descriptions with language-neutral "list-valued arguments." - Add `*Exceptions:*` sections where missing (E, V, hasId, hasKey, hasLabel, hasValue, is, property, where), with `None` body where the step raises no errors and with `Argument Error` bullets where mutating child traversals can be rejected. This brings the new entries into template compliance with the rest of the document. Graph element types in Domain/Range cells (`Vertex`, `Edge`, `VertexProperty`, `Property`, `Element`) are left in PascalCase to match the convention used by the existing step entries on master. Assisted-by: Kiro:claude-opus-4-7
Cole-Greer
force-pushed
the
steps-taking-traversal
branch
from
c726e46 to
bac52b9
Compare
3 participants
Summary
Allow child traversals as arguments to
has(),is(),V(),E(),property(),where(P), and allP/TextPpredicates. The child traversal is evaluated per-traverser at runtime, enabling dynamic value resolution.JIRA:
within()silently return empty instead of executing or erroring - fully addressed (traversals are now executed)V()cannot accept traversals likeselect()for data-driven lookups - fully addressedproperty()should accept a traversal producing a Map - fully addressedhas(key, traversal)doesn't work as expected - partially addressed (dynamic filtering works; runtime folding into index lookups is deferred as a future optimization)What it enables
Review Guide
This is a large change (68 files, ~7k lines) but structured in layers. Review bottom-up:
5f37d11P.resolve(), empty-result semantics,AndPshort-circuit,ConnectiveP/NotPdelegation6a547d2P.resolve().HasStepsingle path,GraphStepidTraversal,AddPropertyStepmapForm,AcceptsChildPredicateTraversalmarkere80423aGremlin.g4nestedTraversalalternatives. Visitors delegate to API (notaddStep())1b21d15within()/without()serialization. .NET typed overloads793e7a8.featurefiles define behavioral contract. Reference doc updates400f5aeArchitecture (single resolution path)
Key decisions to scrutinize
Single resolution path - At the DSL level,
has(key, traversal)wraps the traversal inP.eq(traversal)before creating theHasContainer. This means HasStep only has one code path for traversal resolution: callP.resolve(traverser)on the predicate, thenhc.test(element). The GremlinLang serialization still records the originalhas(key, traversal)form (verified by round-trip tests). This is a new 4.0.0 feature, so no existing provider code is affected. Providers implementing custom strategies must checkHasContainer.hasTraversal()before folding into index lookups.Empty-result semantics - When a child traversal produces zero results, behavior depends on predicate type. Collection predicates (
within/without) resolve to an empty collection and delegate toContains.test()which applies correct set-theoretic logic:within([])is always false,without([])is always true. Scalar predicates (eq/gt/lt/etc.) setresolvedEmpty=trueand steps short-circuit to false since there is no meaningful comparison value. A null result (traversal producesnull) is distinct from an empty result: it resolves normally andnullis used as the comparison value, consistent with existingP.eq(null)semantics.AndP short-circuits, OrP doesn't -
AndP.resolve()overridesConnectiveP.resolve()to stop resolving children at the first scalar predicate that resolves empty. This is sound becauseand(unsatisfiable, X)is always false regardless of X.OrPdoes NOT short-circuit: a non-empty resolution does not guarantee the predicate will passtest(), so later children must already be resolved when their turn comes. Note: short-circuiting means side-effecting child traversals (e.g., ones usingaggregate) in later AndP operands may not execute. This matches standard boolean short-circuit semantics.P.resolve()mutates in place -resolve()overwritesliterals,isCollection, andresolvedEmptyfields on thePinstance. In OLTP this is safe since resolve and test happen sequentially per traverser. In OLAP,HasStep.clone()deep-clones predicates per worker, providing thread isolation. The mutation pattern is a known limitation documented for future improvement (return an immutable resolved value instead). Predicate trees of arbitrary depth (AndP containing OrP containing NotP, etc.) are handled by recursive resolution and recursive cloning.Mutation guard -
ChildTraversalValidatorrejects any child traversal containing a step that implements theMutatinginterface (addV, addE, drop, property). This runs at construction time (DSL methods and P factory methods) and again at strategy time viaChildTraversalVerificationStrategy. Aggregating side-effects (aggregate,store,group) are NOT blocked because they don't modify graph state. Whether they should be restricted is a question for reviewers.Provider contract -
HasContainer.hasTraversal()is the single check point providers must use before folding. Providers that fold HasContainers into index-backed steps without this check will attempt to use an unresolvedPvalue (returnsnullbeforeresolve()is called) as an index key, producing wrong results.TinkerGraphStepStrategydemonstrates the pattern: skip traversal-bearing HasSteps withcontinueso that subsequent literal HasSteps can still be folded. Providers with customGraphStepreplacements must also copy theidTraversalfield during strategy replacement, otherwiseV(traversal)falls through to returning all vertices.choose()restriction -choose(P)andchoose().option(P, ...)reject traversal-bearing predicates at construction time. This is a deliberate scope decision.PredicateTraversal(which evaluates option-key predicates inBranchStep) has access to the traverser and could technically callP.resolve()beforeP.test(), but extending the branching infrastructure is out of scope for this feature. Users needing dynamic branching can use thechoose(Traversal, ...)form where the branch predicate is a full traversal (e.g.,choose(__.is(P.gt(traversal)), trueChoice, falseChoice)). This works becauseIsStephandles resolution normally. Restrictingis()when used insidechoose()'s branch traversal is not feasible sincechoose()receives it as an opaqueTraversal.Adminand does not inspect internal steps.What's NOT in this PR
P.resolve()thread-safety refactoring (future)instanceof DefaultGraphTraversalcleanup (deferred)FilterRankingStrategycost awarenessCompatibility
has(key, traversal)still serializes ashas(key, traversal), verified byGremlinLangTraversalRoundTripTestandGremlinQueryParserTraversalTest.Testing
All
gremlin-coreunit tests pass (9132, 0 failures) and the full TinkerGraph Gherkin suite passes (2172 scenarios, 0 failures) against the currentmaster.HasTraversal.feature,IsTraversal.feature,WhereTraversal.feature,VETraversal.feature,PropertyTraversal.feature,ChildTraversalVerification.featurePTraversalTest,CloneIndependenceTraversalTest,HasContainerTest,ChildTraversalValidatorTest,ChildTraversalVerificationStrategyTestGremlinQueryParserTraversalTest,GremlinLangTraversalRoundTripTestTinkerGraphStepStrategyTraversalTestChecklist
the-traversal.asciidoc)release-4.x.x.asciidoc)mvn clean installpassesVOTE +1